Skip to content

Frozenlist type only 15 #115

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

Frozenlist type only 15 #115

wants to merge 3 commits into from

Conversation

corydolphin
Copy link

Follow up from #113

@Cito would love your thoughts on the best approach. I chose to convert FrozenList to effectively just be an alias for Tuple.

  1. The FrozenList overrides hash (it also looks to be unsafe, in that the hash code would change if you reassigned an element, which is possible)
  2. This allowed the existing isinstance(FrozenList) checks to still exist, while actually making the type immutable.
  3. Having theses objects be truly immutable seems like a good idea, rather than simply expressing it in the type system.

I considered using https://github.com/tobgu/pyrsistent but was hesitant to include another third party library.

@corydolphin corydolphin requested a review from Cito as a code owner October 27, 2020 00:02
@Cito
Copy link
Member

Cito commented Oct 27, 2020

@corydolphin Not sure, need to think about this. There are reasons for using frozen lists or tuples, and there are also arguments for using lists:

  • Enforcing it in run-time does not feel "Pythonic". In Python, a core assumption is that "we are all responsible users". That's why we don't have real private variables for instance.
  • Besides the difference in mutability, traditionally tuples in Python are considered as a replacement for C structs of inhomogenous data, while lists are for homogenous data. In our case we have homogenous data, so typically you would use lists in Python.
  • GraphQL.js also uses ordinary arrays and makes them immutable only via the type.

@corydolphin
Copy link
Author

corydolphin commented Oct 28, 2020

@corydolphin Not sure, need to think about this. There are reasons for using frozen lists or tuples, and there are also arguments for using lists:

  • Enforcing it in run-time does not feel "Pythonic". In Python, a core assumption is that "we are all responsible users". That's why we don't have real private variables for instance.
  • Besides the difference in mutability, traditionally tuples in Python are considered as a replacement for C structs of inhomogenous data, while lists are for homogenous data. In our case we have homogenous data, so typically you would use lists in Python.
  • GraphQL.js also uses ordinary arrays and makes them immutable only via the type.

Agreed. Using tuples doesn't feel very Pythonic, it is a little awkward. That said, they are the only immutable collection provided by the stdlib. In some ways, it is strictly an implementation detail of the library, so we could change it in the future. Without much experience with pyrsistence directly, I'm also not confident in using that library, though it does seem well used and could be a natural alternative providing truly immutable sequences

That said, we currently depend on hashability, so at minimum we need to extend List. If we then allow mutation, it is possible the hash of the object may change, which violates the core contract of hash.

@corydolphin
Copy link
Author

@Cito What do you think the best path forward is? Happy to jump on a zoom/call to discuss potential solutions more

@Cito
Copy link
Member

Cito commented Nov 2, 2020

@corydolphin Thanks for pushing this forward. I'm currently very busy, but will try to look into this again at the end of the week.

pyrsistence looks useful, and there is also immutables, but I would like to keep it as simple as possible and not introduce external dependencies when it is not guaranteed that they are maintained in the long run or when they - as in both of these cases - require compilation. It could cause installation issues on some platforms.

Tuples would be ok (I think I used these in an earlier version already) and would be compatible with the former FrozenList approach, though in my view it might be sufficient if we declare immutability on the type level but not enforce it (yes you could break hashes). I also prefer if we could solve the problem of immutable lists and dicts in a similar/consistent way. Maybe if we decide for tuples, we should also use a frozenmap similar to PEP603 (what's the status of this?), but with a pure Python implementation.

What is your currently preferred solution? Any other idea?

@Cito
Copy link
Member

Cito commented Nov 7, 2020

@corydolphin, are you still working on this (there are some code quality tests still failing)?

I'm currently not sure if this is really a good idea, because the name FrozenList suggests that it is a list, but if you compare such a list to a real list with the same elements, it will create an error because it is really a tuple. This may cause compatibility issues with existing code and break expectations due to the name FrozenList. We should at least overwrite the __eq__ if we want to call it FrozenList. Or we simply use Tuple instead of FrozenList. What do you think?

@corydolphin
Copy link
Author

@corydolphin, are you still working on this (there are some code quality tests still failing)?

I'm currently not sure if this is really a good idea, because the name FrozenList suggests that it is a list, but if you compare such a list to a real list with the same elements, it will create an error because it is really a tuple. This may cause compatibility issues with existing code and break expectations due to the name FrozenList. We should at least overwrite the __eq__ if we want to call it FrozenList. Or we simply use Tuple instead of FrozenList. What do you think?

I agree, with this change we are in a weird halfway of the two worlds which is probably the worst, honestly.
I'll make the change to simply use Tuple.

@corydolphin corydolphin closed this Dec 4, 2020
@corydolphin
Copy link
Author

@Cito sorry, our team ended up working around this by caching validation to improve performance. I'd still like to drive this forward, but this is lower priority than some other changes.

@corydolphin
Copy link
Author

@Cito if you're open to it, I'd like to take a look at improving performance of how execution handles awaitables. In profiling, there is a substantial amount of time spent there.

@Cito
Copy link
Member

Cito commented Dec 4, 2020

Sure, performance improvements are always welcome. My only major concern is that the code stays close to the original GraphQL.js so that we can keep in sync with it and merge in changes from upstream - I have already fallen behind due to lack of time and will need to update a lot again in the next time. If we deviate the logic and flow of code too much from GraphQL.js and do our own thing instead this will become very difficult or impossible.

@corydolphin
Copy link
Author

@Cito There looks to be a substantial performance and complexity win by removing the synchronous execution path and making the execution paths async.
How would you feel about dropping the synchronous execution paths? Non-async resolvers would still be supported of course, but instead of calling the expensive is_awaitable function on every resolved value, and dynamically allocating an additional function when resolving, it would be determined at schema construction time using iscoroutinefunction.

@Cito
Copy link
Member

Cito commented Dec 4, 2020

How compatible would that be? Would everything work as it does now? Also, how urgent is this for you? In the winter holidays I will probably have more time to look deeper into this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants